Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: uart: nrfx_uarte: Major rework of the legacy UART shim to allow using it for nrf54x #75462

Closed

Conversation

nordic-krch
Copy link
Contributor

uart_nrfx_uarte2 shim which is using nrfx_uarte driver has proven to be not a good approach because the split between nrfx and the shim (and nrfx driver supporting additional APIs beyond zephyr one) increases memory footprint and decreases performance and makes it hard to maintain the shim. With significantly increased size it was not feasible to use that shim on cores with small memory (like cpuppr on nrf54h20).

Legacy shim RX asynchronous path had to be reworked because we cannot rely on RXDRDY events when counting bytes because EasyDMA transfers bytes in words so triggering RXDRDY event does not necessary mean that byte is available in RAM.

Rework driver to support new way of asynchronous RX handling. Previously RX was handled in two modes: using RXDRDY interrupt for byte counting or TIMER + PPI. Both modes had flaws. RXDRDY interrupt mode could miscalculated amount of received bytes when interrupt was not handled on time. Data was not lost but was not reported on time and that could lead to issues. PPI+TIMER mode requires additional resources thus it was not the default mode. Often user was not aware of that option and was experiencing driver RX faults.

New RX mode is switching buffers when there is new data (RXDRDY event not set for given amount of time). It does not require additional resources to get precise byte counting. Additionally, this is in line with new UARTE feature (RX frame timeout) which is present in nRF54X devices. The behavior of the driver is the same for legacy devices and new one. For legacy devices k_timer periodic interrupts are used to check if there are any new bytes and it is not needed when RX frame timeout is present.

Improved RX mode is enabled by default (CONFIG_UART_NRFX_UARTE_ENHANCED_RX=y) but legacy modes are still available though not recommended to be used.

Note that new RX mode behaves a bit different because timeout always triggers switch of buffers which means that there will be no UART_RX_RDY events with non-zero offset. It also means that every UART_RX_RDY will be followed by UART_RX_BUF_RELEASED.

Driver structures are reworked and asynchronous data is split into two structures: tx and rx data.

Additionally, support for DMM is added to asynchronous RX. For TX a fixed size, statically allocated buffer is used. Same goes for single byte buffers used for polling and interrupt driven mode (and RX FIFO flush buffer).

After rework, driver is recommended to be used for all platforms as it performs much better (2 times lower CPU load during same data) and takes much less code than the second UART shim available for Nordic devices.

PR contains tests added in #71886 #73638 and size optiomization from #74849.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jul 4, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_nordic DNM This PR should not be merged (Do Not Merge) labels Jul 4, 2024
@nordic-krch nordic-krch force-pushed the uart_legacy_improve2 branch 3 times, most recently from bb8c5e7 to 2f6347f Compare July 8, 2024 18:32
@nordic-krch nordic-krch force-pushed the uart_legacy_improve2 branch 2 times, most recently from c525d64 to b534b84 Compare July 19, 2024 13:52
@zephyrbot zephyrbot removed manifest manifest-hal_nordic DNM This PR should not be merged (Do Not Merge) labels Sep 5, 2024
@nordic-krch nordic-krch force-pushed the uart_legacy_improve2 branch 2 times, most recently from 6b0233e to cb6c784 Compare September 9, 2024 14:45
@nordic-krch nordic-krch marked this pull request as ready for review September 9, 2024 14:46
@zephyrbot zephyrbot added area: UART Universal Asynchronous Receiver-Transmitter area: Samples Samples platform: nRF Nordic nRFx labels Sep 9, 2024
cpuppr is a small core with less than 64k of memory. In order to
fit tests into that memory by default disable asserts (which saves
few kB).

Signed-off-by: Krzysztof Chruściński <[email protected]>
Add test which is using two independent UART devices. Validate behavior of
asynchronous API.

Signed-off-by: Krzysztof Chruściński <[email protected]>
Add configuration for nrf54h20dk/nrf54h20/cpuppr.

Signed-off-by: Krzysztof Chruściński <[email protected]>
Multiple variable in the test were missing static keyword.

Signed-off-by: Krzysztof Chruściński <[email protected]>
Rework the test to be able to run on multiple instances.

Signed-off-by: Krzysztof Chruściński <[email protected]>
Fix target which was failing due to lack of memory for DMA
buffers.

Signed-off-by: Krzysztof Chruściński <[email protected]>
Add second instance to be tested on nrf54h20dk. uart120 is a fast UARTE
which works on fixed pin locations. It is not available for cpuppr core.

Signed-off-by: Krzysztof Chruściński <[email protected]>
Add configuration for nrf54h20dk/nrf54h20/cpuppr.

Signed-off-by: Krzysztof Chruściński <[email protected]>
Rework test to allow testing multiple UART instances in a single
run.

Signed-off-by: Krzysztof Chruściński <[email protected]>
Add configuration for UARTE120 fast instance. Use multi-instance capability
of the test to run same test on UARTE120 and slow UARTE peripheral.

Signed-off-by: Krzysztof Chruściński <[email protected]>
Rework test to be able to support running on multiple
set of instance(s).

Signed-off-by: Krzysztof Chruściński <[email protected]>
Use multi-instance testing and add UARTE120 configuration to nrf54h20
overlays.

Signed-off-by: Krzysztof Chruściński <[email protected]>
Add configurations in which runtime PM is enabled. There are 2
configurations:
- Test does not call PM API for getting and putting the device.
 In that configuration UART device shall be suspended whenever
 not active.
- Test is calling PM API which should keep device active as
 long as requested (e.g. during packet reception which consists
 of multiple chunks).

Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch added a commit to nordic-krch/fw-nrfconnect-zephyr that referenced this pull request Oct 9, 2024
…ache

Add support for DMM which manages cache and dedicated memory spaces.
Added support for data cache for buffers which are not DMM managed.

Upstream PR: zephyrproject-rtos/zephyr#75462

Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch added a commit to nordic-krch/fw-nrfconnect-zephyr that referenced this pull request Oct 9, 2024
…karound

Add configurable magic byte instead of fixed 0xAA. In some cases, it
is known that certain bytes are less likely in the transmission and
picking specific magic byte may reduce probability of failure of
detection of the correct amount of flushed data.

Upstream PR: zephyrproject-rtos/zephyr#75462

Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch added a commit to nordic-krch/fw-nrfconnect-zephyr that referenced this pull request Oct 9, 2024
…TE register

On uart120 BAUDRATE register is not retained when ENABLE=0 and
because of that BAUDRATE must be set after enabling. Add workaround
to the driver.

Upstream PR: zephyrproject-rtos/zephyr#75462

Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch added a commit to nordic-krch/fw-nrfconnect-zephyr that referenced this pull request Oct 9, 2024
Add lock to fix race when uart_rx_disable is interrupted by RXTO
event which lead to driver state corruption.

Upstream PR: zephyrproject-rtos/zephyr#75462

Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch added a commit to nordic-krch/fw-nrfconnect-zephyr that referenced this pull request Oct 9, 2024
Add runtime PM to the driver. When asynchronous or interrupt
driven API is used, there are 3 independent paths for enabling
the device: RX, TX and TX poll_out. TX poll_out requires special
handling because number of poll_out calls does not need to much
number of TXSTOPPED interrupts. To handle that special flag is
added. For standard RX and TX is simpler.

Upstream PR: zephyrproject-rtos/zephyr#75462

Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch added a commit to nordic-krch/fw-nrfconnect-zephyr that referenced this pull request Oct 9, 2024
So far new platforms did not use legacy shim but it shall
be now a default uart shim for all platforms.

Upstream PR: zephyrproject-rtos/zephyr#75462

Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch added a commit to nordic-krch/fw-nrfconnect-zephyr that referenced this pull request Oct 11, 2024
…ache

Add support for DMM which manages cache and dedicated memory spaces.
Added support for data cache for buffers which are not DMM managed.

Upstream PR: zephyrproject-rtos/zephyr#75462

Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch added a commit to nordic-krch/fw-nrfconnect-zephyr that referenced this pull request Oct 11, 2024
…karound

Add configurable magic byte instead of fixed 0xAA. In some cases, it
is known that certain bytes are less likely in the transmission and
picking specific magic byte may reduce probability of failure of
detection of the correct amount of flushed data.

Upstream PR: zephyrproject-rtos/zephyr#75462

Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch added a commit to nordic-krch/fw-nrfconnect-zephyr that referenced this pull request Oct 11, 2024
…TE register

On uart120 BAUDRATE register is not retained when ENABLE=0 and
because of that BAUDRATE must be set after enabling. Add workaround
to the driver.

Upstream PR: zephyrproject-rtos/zephyr#75462

Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch added a commit to nordic-krch/fw-nrfconnect-zephyr that referenced this pull request Oct 11, 2024
Add lock to fix race when uart_rx_disable is interrupted by RXTO
event which lead to driver state corruption.

Upstream PR: zephyrproject-rtos/zephyr#75462

Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch added a commit to nordic-krch/fw-nrfconnect-zephyr that referenced this pull request Oct 11, 2024
Add runtime PM to the driver. When asynchronous or interrupt
driven API is used, there are 3 independent paths for enabling
the device: RX, TX and TX poll_out. TX poll_out requires special
handling because number of poll_out calls does not need to much
number of TXSTOPPED interrupts. To handle that special flag is
added. For standard RX and TX is simpler.

Upstream PR: zephyrproject-rtos/zephyr#75462

Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch added a commit to nordic-krch/fw-nrfconnect-zephyr that referenced this pull request Oct 11, 2024
So far new platforms did not use legacy shim but it shall
be now a default uart shim for all platforms.

Upstream PR: zephyrproject-rtos/zephyr#75462

Signed-off-by: Krzysztof Chruściński <[email protected]>
CONFIG_ZTEST_THREAD_PRIORITY=5
CONFIG_MAIN_STACK_SIZE=2048
CONFIG_ZTEST_STACK_SIZE=2048
CONFIG_TEST_RANDOM_GENERATOR=y
CONFIG_COUNTER=y

# CONFIG_NO_OPTIMIZATIONS=y
CONFIG_OUTPUT_DISASSEMBLY=y
#CONFIG_NO_OPTIMIZATIONS=y
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some of configs here can be removed. Additionally, do we need disassembly output here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR becomes an superset of all changes (uart + tests). I will be opening clean PR with only driver changes (PM) since that is what we need now. Tests will come later with less pressure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Logging area: Samples Samples area: UART Universal Asynchronous Receiver-Transmitter platform: nRF Nordic nRFx
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants